Add (configurable) guards for complexity of expressions - #16423
Conversation
…ParseException instead of StackOverflowError or LinkageErrors
| * complexity) | ||
| */ | ||
| public static Expression compile( | ||
| String sourceText, Map<String, MethodHandle> functions, int maxNestingDepth) |
There was a problem hiding this comment.
do we need to make the nestingDepth configurable? Given that this is a safety check, 250 really seems like plenty
There was a problem hiding this comment.
Actually if you read the whole thing, you may notice that this not only affects "nesting" of expressions. Also a long "1 + 2 + 3 + 4 + ..... + 10000" will trigger the failure. I am not sure if 250 is enough for all cases, so I'd like to have it configurable (at least for a while, so people can tune it).
There was a problem hiding this comment.
But I agree, if we can remove it and have a sane default - I am fine. But if we don't want to have it, catching "StackOverFlowException" might be enough, too.
There was a problem hiding this comment.
hm, I did not realize a single expression with lots of + would be included. You know at one time we attempted to use Lucene's expression language to encode machine-learned decision trees that had thousands and thousands of nodes. Ultimately we concluded this was bad idea and implemented a custom handling function,, but the the reason was not StackOverflow, but some internal limit in the JDK on byte code. I'm forgetting exactly what was limited - maybe the total size of byte code, number of instructions, or number of identifiers? Anyway there can be cases with very large expressions, so I get your point that 250 may not be enough. Still, this feels something like BooleanQuery's 1024 limit or vector search's 1024-dimension limit - can we pick a large default (maybe 1024 is our magic number)? But I don't fundamentally object to exposing the config parameter, just wonder if anyone would use it
There was a problem hiding this comment.
Yeah, this is now also handled in this PR. If the JVM refuses the class file due to internal limitations, an IllegalStateException is thrown, too (instead of an error).
There may also be other limitations in the JVM that could cause a stack overflow also at runtime. Basically those two "identical" expressions (semantically), will cause a different bytecode. The first and second one have same AST and are more optimal, but the third one consumes much more space during execution:
a + b + c + d: bytecode: push a, push b, add, push c, add, push d, add((a + b) + c) +d: bytecode: push a, push b, add, push c, add, push d, adda + (b + (c + d)): bytecode push a, push b, push c, push d, add, add, add
The last one pushes all arguments onto stack and calls three times "add". This is consuming more stack than the first two variants (with same AST). But as first and second are identical in the AST, you see why the first one has implicit precedence included and therefor may cause a stack overflow during parsing.
You see it is complicated and therefor a hardcoded nesting limit, but possibly also a "number of tokens" limit should be enforced (configurable).
The number of tokens limit can be added, if we agree on it.
I hope this helps to understand what's going on!
There was a problem hiding this comment.
Anyways maybe 1024 is a better default limit! In my testing 2048 sometimes causes stack overflows already (especially in testing environment while the "picky" mode is enabled).
There was a problem hiding this comment.
I updated javadocs to clarify what nesting depth means.
|
This can be backported if needed (it does not depend on the ASM vs. Classfile library used for compiling. It may just create minor merge conflicts. |
rmuir
left a comment
rmuir
left a comment
There was a problem hiding this comment.
I think its fine.
alternative names might be "max depth" or "max tree depth" but "max nesting depth" isn't the worst and will work.
|
Thanks for review, I added a small improvement to not traverse the tree two times. The depth check is now done inside the visitor! |
* Add (configurable) guards for complexity of expressions to fail with ParseException instead of StackOverflowError or LinkageErrors * Raise nesting limit to 1024, add documentation that implicit precedence also counts * Avoid 2nd recursive tree walking by doing it while building classfile * add changes entry * add extra test (on boundary) # Conflicts: # lucene/CHANGES.txt # lucene/expressions/src/java/org/apache/lucene/expressions/js/JavascriptCompiler.java
|
Backport was possible with few modifications (mostly test annotation and some conflicts): 1dc74b9 |
3 participants
...so we fail withParseException instead of StackOverflowError or LinkageErrors when the expression is too complex.
Description
Any application that compiles caller-influenced expressions through
JavascriptCompilerinherits an undeclared crash path. BecauseStackOverflowErroris aVirtualMachineError, generictry/catch (ParseException)or evencatch (Exception)aroundcompile()does not contain it; only an explicitcatch (StackOverflowError)/catch (Error)does.Concrete downstream example: Elasticsearch exposes the expressions language as a scripting engine. A single small request containing a nested-parenthesis expression reaches
JavascriptCompiler.compile(), theStackOverflowErrorescapes Elasticsearch'sParseException-only handler, and Elasticsearch's fatal-error handler halts the JVM. This is partly an Elasticsearch defense-in-depth gap (it catches this same error for its Painless engine but not for expressions), and that is being addressed separately with Elasticsearch.The stopped JVM is actually not a bug in Lucene (Elasticsearch kills itsself when the stack overflows or on any other error), but as this is partly caused by the ANTLR lexr/parser to work recursive, this should be handled.
This PR adds a configurable limit on the nesting in Lucene expressions (defaults to
2501024) which is checked on parsing and building theParseTree-- but also adds a "last safety": When a StackOverflowError happens during building the final class file, the parsing fails with aParseException, too. If others think the additional listener is too much overhead, we can remove the configurable limit and only trigger on a stack overflow.The configurable limit has the pro that it is not JVM dependent. An expression will fail consistently with a given limit and not depending on the call stack and config of the JVM. This may also be used by software like Elasticsearch to limit the complexity of expressions. We may also add a limit on how many method calls are allowed at a later stage.
The PR also adds another catch for
LinkageErrorthat is rethrown as (documented)IllegalStateExceptionif the parser created a class file which does not load at all (e.g., due to the size of bytecode or a bug in our parser).It also updates documentation to clarfiy which exceptions are thrown.
Thansk to @skraft9 for the hint about this problem!